Moved ghost/core boundary enforcement to dependency-cruiser#29134
Conversation
|
Bugbot is not enabled for this team, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughThis PR replaces ESLint-based architectural boundary enforcement with dependency-cruiser. It adds a Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
package.json (1)
63-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider
err-longoutput for clearer boundary-violation diagnostics.Default depcruise output is terse;
--output-type err-longincludes the rule'scomment/namein the failure output, which would help contributors working through the documented allowlist ("Goal: Work down until only proxy.js remains").♻️ Optional diff
- "lint:boundaries": "depcruise ghost/core/core --config .dependency-cruiser.cjs", + "lint:boundaries": "depcruise ghost/core/core --config .dependency-cruiser.cjs --output-type err-long",Also applies to: 122-124
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@package.json` at line 63, The lint:boundaries depcruise script currently uses terse default output, so update the lint:boundaries command to use err-long for clearer boundary-violation diagnostics. Adjust the package.json script that runs depcruise so contributors get the rule comment/name in failures, making it easier to work through the allowlist guidance and locate violations in ghost/core/core.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@package.json`:
- Line 63: The lint:boundaries depcruise script currently uses terse default
output, so update the lint:boundaries command to use err-long for clearer
boundary-violation diagnostics. Adjust the package.json script that runs
depcruise so contributors get the rule comment/name in failures, making it
easier to work through the allowlist guidance and locate violations in
ghost/core/core.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 17b3d2f3-731d-441a-9bc9-a04fc284a5f9
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
.dependency-cruiser.cjs.github/workflows/ci.yml.lintstagedrc.cjsghost/core/eslint.config.mjspackage.jsonpnpm-workspace.yaml
💤 Files with no reviewable changes (1)
- ghost/core/eslint.config.mjs
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx run @tryghost/admin-x-settings:test:acceptance |
✅ Succeeded | 11m 28s | View ↗ |
nx run-many --target=build --projects=tag:publi... |
✅ Succeeded | <1s | View ↗ |
nx run ghost:test:ci:integration |
✅ Succeeded | 2m 49s | View ↗ |
nx run-many -t test:unit -p @tryghost/kg-markdo... |
✅ Succeeded | 8m 22s | View ↗ |
nx run ghost:test:integration |
✅ Succeeded | 2m 51s | View ↗ |
nx run @tryghost/admin:build |
✅ Succeeded | 3m 24s | View ↗ |
nx run ghost:test:legacy |
✅ Succeeded | 3m 24s | View ↗ |
nx run ghost-monorepo:lint:boundaries |
✅ Succeeded | 6s | View ↗ |
Additional runs (11) |
✅ Succeeded | ... | View ↗ |
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗
☁️ Nx Cloud last updated this comment at 2026-07-09 15:35:52 UTC
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #29134 +/- ##
==========================================
- Coverage 74.07% 74.07% -0.01%
==========================================
Files 1570 1570
Lines 136629 136628 -1
Branches 16530 16528 -2
==========================================
- Hits 101204 101202 -2
+ Misses 34419 34390 -29
- Partials 1006 1036 +30
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
6c67989 to
558830a
Compare
|
Bugbot is not enabled for this team, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
no ref The frontend/server/shared boundaries were enforced by the custom `ghost/node/no-restricted-require` rule in ghost/core/eslint.config.mjs. That rule is specific to eslint-plugin-ghost and has no equivalent in oxlint, so it blocks the planned ESLint-to-oxlint migration. It also only inspects require() calls, missing ESM imports. The four boundary rules now run on dependency-cruiser, which enforces them on the resolved module graph (require and import) from the repo root. This drops the custom rule and lets the boundaries outlive ESLint. The original burn-down intent — "adding files to this list is an anti-pattern / work down until the list is empty" — and the per-file allowlist categorisation are carried over verbatim from the pre-flat- config .eslintrc.js, having been lost in the flat-config migration. Catching imports as well as requires immediately surfaced one leak the old rule missed: route-settings/activation-bridge.ts imports the frontend routing config, the same crossing validate.js already makes via require(). It is allowlisted alongside validate.js as known, soon-to-be-removed debt. Boundaries stay part of the single `pnpm lint` command: it runs run-many across every eslint lint target plus the boundaries target, so there is no separate lint command to remember. The boundaries target is a cached nx target on the ghost-monorepo root project (scoped inputs so the whole-repo projectRoot glob doesn't defeat caching); it also runs unconditionally in CI and as a lint-staged pre-commit signal. A top-level `pnpm check` runs lint plus tests. The cross-package apps/library model is deliberately out of scope and tabled as a follow-up.
558830a to
f70e1cd
Compare
|
Bugbot is not enabled for this team, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |

Why
ghost/core's frontend/server/shared layer boundaries are enforced by the customghost/node/no-restricted-requirerule inghost/core/eslint.config.mjs. That rule is specific toeslint-plugin-ghostand has no equivalent in oxlint, so it stands in the way of the planned ESLint → oxlint migration. It also only inspectsrequire()calls, so it misses ESMimports.What
Moves the four boundary rules onto dependency-cruiser, which enforces them on the resolved module graph (both
requireandimport) from the repo root:core/sharedmust not depend oncore/serverorcore/frontendcore/frontendmust not reachcore/server/modelsdirectlycore/frontendmay cross tocore/serveronly via theservices/proxyseam (allowlisted)core/servermust not depend oncore/frontend(allowlisted)The custom
no-restricted-requireblocks (and their now-unusedpath/__dirnameimports) are removed from the ESLint config. The original burn-down intent — "adding files to this list is an anti-pattern / work down until the list is empty" — and the per-file allowlist categorisation are carried over verbatim from the pre-flat-config.eslintrc.js(they'd been dropped in the flat-config migration).Enforced in three places:
ghost-monoreporoot project (lint:boundaries), with scopedinputsso the root project's whole-repoprojectRootglob doesn't defeat cachingghost/core/corefilesBehaviour is unchanged: parity verified (0 violations on the same graph the ESLint rules covered), and each rule still fires on a negative test.
Scope
The cross-package
apps/*boundary model (foundation-libraries vs apps) surfaced during this work but is deliberately out of scope here and tabled as a follow-up.Checklist